Skip to content

fix: ignore late ICE candidates after cleanup for RN speech detector#2193

Merged
santhoshvai merged 1 commit intomainfrom
rn-speech-detector
Apr 9, 2026
Merged

fix: ignore late ICE candidates after cleanup for RN speech detector#2193
santhoshvai merged 1 commit intomainfrom
rn-speech-detector

Conversation

@santhoshvai
Copy link
Copy Markdown
Member

@santhoshvai santhoshvai commented Apr 9, 2026

💡 Overview

Properly remove event listeners early in RN-speech-detector and avoid sending invalid candidates to native side

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced reliability of speech detection cleanup and shutdown behavior with more robust event listener management.
    • Improved error handling and logging for ICE candidate forwarding operations.
    • Fixed potential issues with repeated shutdown attempts through idempotent stop behavior.
  • Tests

    • Added comprehensive test coverage for speech detection listener lifecycle and cleanup.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 9, 2026

⚠️ No Changeset found

Latest commit: 3634c78

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@santhoshvai santhoshvai changed the title Ignore late ICE candidates after cleanup for RN speech detector fix: Ignore late ICE candidates after cleanup for RN speech detector Apr 9, 2026
@santhoshvai santhoshvai changed the title fix: Ignore late ICE candidates after cleanup for RN speech detector fix: ignore late ICE candidates after cleanup for RN speech detector Apr 9, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 9, 2026

📝 Walkthrough

Walkthrough

The RNSpeechDetector class is refactored to use readonly peer connection fields, add an isStopped idempotency guard, and replace inline event listeners with named handlers. A new forwardIceCandidate() helper method centralizes ICE candidate forwarding with control-flow checks and improved error handling. Event listener cleanup is now properly symmetrical with listener registration.

Changes

Cohort / File(s) Summary
Event Listener Refactoring
packages/client/src/helpers/RNSpeechDetector.ts
Peer connection fields are now readonly. Introduces isStopped flag for idempotent shutdown. Converts inline icecandidate and track event listeners into named handler methods. Extracts ICE candidate forwarding into forwardIceCandidate() helper with control-flow checks (skip if stopped, candidate is null, or destination signaling state is closed). Improves error handling by catching addIceCandidate failures and logging info messages. Event listener cleanup now explicitly removes the same handlers that were registered.
Test Suite
packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts
New Vitest test suite verifying that cleanup function properly removes icecandidate event listeners registered during start(). Test manually invokes registered ICE event handler, confirms removeEventListener was called, and asserts addIceCandidate was not invoked on the remote peer connection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Hoppity-hop, the handlers align,
Named listeners now, no longer inline,
Ice candidates flow with guarding so neat,
Cleanup and stops make the refactor complete! ❄️✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description covers the overview but is missing implementation notes and ticket/docs references specified in the repository template. Add Implementation notes section and complete Ticket/Docs links to match the repository's description template.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title directly describes the main purpose of the changeset: preventing late ICE candidates from being processed after cleanup in the RN speech detector.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rn-speech-detector

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/client/src/helpers/RNSpeechDetector.ts (1)

20-70: ⚠️ Potential issue | 🟠 Major

Ensure partial start() failures run full cleanup.

If any step throws after Line 43 listener registration or after Line 56 subscription setup, the catch block (Line 66) only logs and returns a noop, leaving resources/listeners active.

Proposed fix
 public async start(onSoundDetectedStateChanged: SoundStateChangeHandler) {
+  let onPc1IceCandidate:
+    | ((e: RTCPeerConnectionIceEvent) => void)
+    | undefined;
+  let onPc2IceCandidate:
+    | ((e: RTCPeerConnectionIceEvent) => void)
+    | undefined;
+  let onTrackPc2: ((e: RTCTrackEvent) => void) | undefined;
+  let unsubscribe: (() => void) | undefined;
   try {
     this.isStopped = false;
@@
-    const onPc1IceCandidate = (e: RTCPeerConnectionIceEvent) => {
+    onPc1IceCandidate = (e: RTCPeerConnectionIceEvent) => {
       this.forwardIceCandidate(this.pc2, e.candidate);
     };
-    const onPc2IceCandidate = (e: RTCPeerConnectionIceEvent) => {
+    onPc2IceCandidate = (e: RTCPeerConnectionIceEvent) => {
       this.forwardIceCandidate(this.pc1, e.candidate);
     };
-    const onTrackPc2 = (e: RTCTrackEvent) => {
+    onTrackPc2 = (e: RTCTrackEvent) => {
@@
-    const unsubscribe = this.onSpeakingDetectedStateChange(
+    unsubscribe = this.onSpeakingDetectedStateChange(
       onSoundDetectedStateChanged,
     );
     return () => {
@@
   } catch (error) {
+    if (onPc1IceCandidate) {
+      this.pc1.removeEventListener('icecandidate', onPc1IceCandidate);
+    }
+    if (onPc2IceCandidate) {
+      this.pc2.removeEventListener('icecandidate', onPc2IceCandidate);
+    }
+    if (onTrackPc2) {
+      this.pc2.removeEventListener('track', onTrackPc2);
+    }
+    unsubscribe?.();
+    this.stop();
     const logger = videoLoggerSystem.getLogger('RNSpeechDetector');
     logger.error('error handling permissions: ', error);
     return () => {};
   }
 }

As per coding guidelines: "Unregister event listeners in cleanup/disposal code to prevent memory leaks".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/client/src/helpers/RNSpeechDetector.ts` around lines 20 - 70, The
try block can throw after listeners (onPc1IceCandidate, onPc2IceCandidate,
onTrackPc2) and after subscription (unsubscribe) are created, but the catch only
logs and returns a noop leaving listeners, tracks and peers active; add a
deterministic cleanup path and call it from the catch (or use finally).
Implement a local cleanup routine inside start() that removes the three
listeners from this.pc1/this.pc2, calls unsubscribe() if it was set, stops and
nulls audioStream tracks, and calls this.stop() / closes pc1/pc2 as appropriate,
then invoke that cleanup from the catch so partial failures free resources and
prevent leaks.
🧹 Nitpick comments (1)
packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts (1)

31-50: Extend this regression to cover both ICE forwarding directions.

Great regression test for pc1 -> pc2. Since the implementation also wires pc2 -> pc1, add a symmetric assertion for the reverse handler to prevent half-fixed regressions.

Based on learnings: "Add regression tests for bug fixes".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts` around lines
31 - 50, The test currently simulates a late ICE candidate from pc1 and asserts
it was removed and not forwarded to pc2; extend it symmetrically for pc2->pc1 by
locating the second registered ICE callback from
rtcPeerConnectionMockCtor.mock.results (use the same pattern that finds
onIceCandidate), invoke that callback with a fake ICE candidate, then assert
pc2.removeEventListener was called with that handler and that
pc1.addIceCandidate was not called; reference rtcPeerConnectionMockCtor, pc1,
pc2, addEventListener, removeEventListener, and addIceCandidate when making the
additions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/client/src/helpers/RNSpeechDetector.ts`:
- Around line 20-70: The try block can throw after listeners (onPc1IceCandidate,
onPc2IceCandidate, onTrackPc2) and after subscription (unsubscribe) are created,
but the catch only logs and returns a noop leaving listeners, tracks and peers
active; add a deterministic cleanup path and call it from the catch (or use
finally). Implement a local cleanup routine inside start() that removes the
three listeners from this.pc1/this.pc2, calls unsubscribe() if it was set, stops
and nulls audioStream tracks, and calls this.stop() / closes pc1/pc2 as
appropriate, then invoke that cleanup from the catch so partial failures free
resources and prevent leaks.

---

Nitpick comments:
In `@packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts`:
- Around line 31-50: The test currently simulates a late ICE candidate from pc1
and asserts it was removed and not forwarded to pc2; extend it symmetrically for
pc2->pc1 by locating the second registered ICE callback from
rtcPeerConnectionMockCtor.mock.results (use the same pattern that finds
onIceCandidate), invoke that callback with a fake ICE candidate, then assert
pc2.removeEventListener was called with that handler and that
pc1.addIceCandidate was not called; reference rtcPeerConnectionMockCtor, pc1,
pc2, addEventListener, removeEventListener, and addIceCandidate when making the
additions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 816bdcfc-bab2-48dd-a4e2-b693bc268c18

📥 Commits

Reviewing files that changed from the base of the PR and between b54f68f and 3634c78.

📒 Files selected for processing (2)
  • packages/client/src/helpers/RNSpeechDetector.ts
  • packages/client/src/helpers/__tests__/RNSpeechDetector.test.ts

@santhoshvai santhoshvai merged commit f8735d6 into main Apr 9, 2026
19 of 20 checks passed
@santhoshvai santhoshvai deleted the rn-speech-detector branch April 9, 2026 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants